fix: Issue #970: interpreter called only when contract present #1047

jeshecdom · 2024-11-15T11:04:20Z

Issue

Closes #970.

Solves the issue by adding a new step in the build pipeline: the tact optimization phase immediately after typechecking (i.e., immediately after precompile).

Currently, the optimization phase only carries out expression simplification in the AST by running the interpreter over all expressions in the AST (later, this will be replaced by the partial evaluator). I did this in preparation for other optimization techniques that will be added in the future (for example, constant propagation will be moved to the optimization phase later).

The PR adds two CLI compiler options:

skipTactOptimizationPhase: Default is false. If true, skips the tact optimization phase. Skipping the phase is useful, for example, to carry out tests in which you want to deactivate the interpreter so that you work with the raw program. Simply add the option in your project's json:

"projects": [
    {
      "name": "test",
      "path": "./test.tact",
      "output": "./output",
      "options": { 
          "skipTactOptimizationPhase": true 
      }
    }
  ]

dumpOptimizedTactCode: Default is false. If true, it will produce two files: one with the program before the optimization phase, and another one after the optimization phase, so that you can make a diff on both files and see what the optimization phase did to the program. Note that this option is ignored if the optimization phase was skipped.

NOTE: This PR does not modify the calls to the interpreter that are currently done in the FunC generator code. This means that the Tact interpreter is unnecessarily called twice. These second calls to the interpreter will be removed later in a separate issue.

NOTE 2: This PR does not modify the AST one gets from calling getRawAST on the context object ctx. I did a quick check and the FunC generation part seems to make use of the functions getAllTypes, getAllStaticFunctions and the like. This PR DOES modify the ASTs produced by those functions. The FunC generator part seems to use only the "sources" from getRawAST, but I still do not understand that part of the code.

Checklist

I have updated CHANGELOG.md
I have added tests to demonstrate the contribution is correctly implemented: this usually includes both positive and negative tests, showing the happy path(s) and featuring intentionally broken cases
I have run all the tests locally and no test failure was reported
I have run the linter, formatter and spellchecker
I did not do unrelated and/or undiscussed refactorings

TODOs, like adding the CLI option to activate/deactivate optimization phase.

Added CLI options for skipping optimization phase and for dumping optimized tact code.

Further fixes: - subexpressions inside struct instances did not have their types registered correctly. - makeValueExpression now receives a SrcInfo object.

verytactical · 2024-11-26T21:48:47Z

schemas/configSchema.json

+              "dumpOptimizedTactCode": {
+                "type": "boolean",
+                "default": false,
+                "description": "False by default. If set to true, dumps the code produced by the Tact code optimization phase. In case the optimization phase is skipped, this option is ignored."


"Smooth" (in math sense) behavior would be to dump the code anyway, yet unoptimized. "Dump the optimized code" is just another separate optional compilation step that happens to be in the list after "optimize" step.

verytactical · 2024-11-26T21:49:06Z

src/interpreter.ts

@@ -960,7 +960,7 @@ export class Interpreter {
                if (foundContractConst.value !== undefined) {
                    return foundContractConst.value;
                } else {
-                    throwErrorConstEval(
+                    throwNonFatalErrorConstEval(


Sorry I don't understand this change, and I suspect a future reader wouldn't either.

When do we throw non-fatal errors in general?

When do we throw non-fatal errors in general?

When we want to stop the constant evaluation, but continue compiling the rest of the contract. Like, when not being able to compute something at compile-time with ability to fallback to run-time implementation for that something.

stopEvaluation()?

Yeah, but the current pipeline kinda relies on throwing and catching errors and not on errors as values ¯\_(ツ)_/¯

verytactical · 2024-11-26T21:49:27Z

src/optimizer/optimization_phase.ts

+    return ctx;
+}
+
+export function dump_tact_code(ctx: CompilerContext, file: string) {


It's odd that the function calls prettyPrint several times.

prettyPrint is supposed to print the whole AST node, whatever its type is.

Either we're missing another type of AST node, or this is actually something in the lines of prettyPrint({ kind: 'module', ... }).

verytactical · 2024-11-26T21:49:37Z

src/optimizer/optimization_phase.ts

+        program += `${prettyPrint(t.ast)}\n\n`;
+    }
+
+    writeFileSync(file, program, {


writeFile does w by default, doesn't it?

Also *Sync functions are synchronous and stop the whole JS virtual machine until they're done with what they were doing. It's fine to use a few of these in tests or in build scripts, but I think they can never be used from any of src/**/*.ts.

Eventually we might want to read several files during compilation, and in case of Web IDE probably even download those files over wire. *Sync is impossible to emulate in web (barring some unfortunate deprecated APIs that make the whole browser tab unresponsive), and there'll be no easy way to unSync the code in the future.

This should probably use import fs from "node:fs/promises"

verytactical · 2024-11-26T21:49:54Z

src/optimizer/test/__snapshots__/expr_simplification.spec.ts.snap

@@ -0,0 +1,184 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`expression-simplification should fail expression simplification for interpreter-called-when-no-contract 1`] = `


Very, very wise use of Jest snapshots.

I was thinking to investigate how to do this, but you've already done it. Thanks!

verytactical · 2024-11-26T22:03:43Z